-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ICollection/Layer): Group Rewrite #7473
Conversation
* Create layer.class.js * Update build.js * Update layer.class.js * stable * Update layer.class.js * add/remove objects * Update layer.class.js * svg * typo * Update layer.class.js * fix(): remove + layout * fix(): layout * svg * Update layer.class.js * Update layer.class.js * lint * Update canvas.class.js * Update layer.class.js * Revert "svg" This reverts commit 1433f56. * perf(): `SprayBrush` case `SprayBrush` case - a huge amount of objects optimizing logic according to `subTargetCheck` * parent property * Update layer.class.js * Update layer.class.js * feat(Object#onSelect): support returning a descendant of object Return a descendant of object (`subTargets` prop of the first argument passed to `onSelect`). This makes selecting a child object of a collection possible. * _applyLayoutStrategy: add context + call on object change * fix(_setActiveObject): handle nested selection if object is already selected * fix(): call setCoords, getObjectsBoundingBox * Update canvas.class.js * ci(): lint * Update canvas_events.mixin.js * better `getObjectsBoundingBox` * Revert "Update canvas_events.mixin.js" This reverts commit 025a76f. * Update canvas.class.js * Update layer.class.js * Update canvas_events.mixin.js * Update canvas_events.mixin.js * Update layer.class.js * Update layer.class.js * Update object_stacking.mixin.js * Update active_selection.class.js * Update layer.class.js * Update layer.class.js * Update layer.class.js * Update layer.class.js * fix(): render selected on top * lint * Update layer.class.js * fix(): nested selection * Update layer.class.js * fix(): selection stack order selecting an object belonging to the same parent while under a foreign object. * Update layer.class.js * fix(): caching
Changed |
Last commits optimized layer in such a way I think it is now more performant than group.
|
src/parser.js
Outdated
@@ -29,6 +29,7 @@ | |||
display: 'visible', | |||
visibility: 'visible', | |||
transform: 'transformMatrix', | |||
'fabric-layout': 'layout', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
i m starting to review and taking notes. Can you leave the code steady for a bit? |
Yes.
בתאריך יום ה׳, 2 בדצמ׳ 2021, 13:52, מאת Andrea Bogazzi <
***@***.***>:
… i m starting to review and taking notes. Can you leave the code steady for
a bit?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#7473 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIGAW4P4P5SNUINKL7Q4THTUO5MW5ANCNFSM5HLK6WLQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
There is some good stuff here! |
I will work with PRs and merge into this one so you won't get overwhelmed with commits or changes while trying to review this. |
_setActiveObject: function(object, e) { | ||
if (this._activeObject === object) { | ||
_setActiveObject: function (object, e) { | ||
var isCollection = Array.isArray(object._objects), activeObject = this._activeObject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For fixing the drag of multiple object isCollection should be fixed and line 1151 should be
var isCollection = (Array.isArray(object._objects) && object.type != 'activeSelection'), activeObject = this._activeObject;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll look into it.
Just wanted to make sure you've tested this with #7529 merged into your local branch.
This should do it
git fetch upstream
git merge ShaMan123/layer-proposal-patch --squash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test it but I have a few thoughts.
If I understand correctly if we accept your proposal selecting objects nested in active selection will become impossible.
Can you confirm?
If so this case should be handled by the dev overriding onSelect
of active selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me its working as it should selecting in icollection when its there but doesnt cancel selection for active selection, tested on #7529 merged branch.
* getAncestors * isInFrontOf * tests * fix(Canvas): `_chooseObjectsToRender` * Update object.js * Update object.js
@ebigtm says:
|
closing this in favor of #7669 |
Motivation
#7316 #6776 #7136 #7299 #7130 #7142
Group is obsolete. We need something much better for fabric.
I think this is a successful predecessor that can replace Group entirely.
This PR aims first and foremost to enable/support nested selection under an Object.
Gist
I managed to come up with this solution thanks to this fabricjs/fabricjs.com#315:
This is why Layer objects are transformed relative to canvas
Impl
To Do
objectCaching=false
: need a better hook for_applyMatrixDiff
getLayoutStrategyResult
)getLayoutStrategyResult
with clip path?layout
prop, transform, subTargetCheckAdditional
SprayBrush
from group?